Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Natalie Tan] Duke Increments #344

Open
wants to merge 96 commits into
base: master
Choose a base branch
from

Conversation

nattanyz
Copy link

@nattanyz nattanyz commented Sep 2, 2019

No description provided.

Copy link

@gary-lgy gary-lgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Natalie!
Basic functionalities are implemented. Comments are helpful for understanding the code.
Please refer to the individual comments for the specifics.
Don't forget to finish the leftover increments in time ;)

@@ -1,10 +1,10 @@
import java.util.Scanner;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import :)

public class DukeBot {
private Scanner in;
private TaskList taskList = new TaskList();
private static final String welcomeMessage = "What can I do for you?";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the coding standard:

Constant names must be all uppercase using underscore to separate words.

* Starts DukeBot and prints welcome message.
*/

public void initialise() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These code should be in the constructor and run can be a public method that can be called separately IMO.

*/

public void initialise() {
String logo = " ____ _ \n"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be a constant too.

input = in.nextLine();
if (input.equals("bye")) {
// if "bye", terminate
// todo: is there a way to combine this into processInput()?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can make processInput return a boolean to indicate whether the command is "bye"

*
* @param input input string given by user.
*/
public void processInput(String input) throws InputMismatchException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having almost all the app logic here, maybe it would help to define a Command class and a few subclasses like AddCommand, DoneCommand, etc. It would make the code more maintainable.


public void printList() {
int i = 1;
for (Iterator<Task> iterator = this.tasks.iterator(); iterator.hasNext(); i++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the iterator here is necessary :)

tasks.add(newTask);
System.out.println("Okay! I've added: " + description
+ ". Use list to see all your tasks!");
// todo: description has trailing space due to using /at or /by as delimiter. how?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use strip(), stripLeading(), stripTrailing(), whichever that suits your need :)

TODO
}

public void addToList(TaskType taskType, String description, String deadline)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this method take in a Task and construct the Task at the call site. This reduces duplicate code.

}
break;
case "event":
inputReader.useDelimiter("/at");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty neat :)

@@ -1,10 +1,10 @@
import java.util.Scanner;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

…geBox to represent different types of message boxes. change Gui class to use these subclasses.
* 'master' of https://github.com/nattanyz/duke: (22 commits)
  remove redundant print statements in Task class, change filePath for GuiDuke, build Jar file
  remove System.out.println statements. add SaveCommand
  add showUserInput() method to Gui class
  add DukeMessageBox, ExceptionMessageBox, TaskMessageBox and UserMessageBox to represent different types of message boxes. change Gui class to use these subclasses.
  add Storage param back to execute() method in all Commands, add storage.save() when ExitCommand() is issued
  use ui.showMessage() in Storage class, add try/catch blocks to debug, manage to get partially working GUI (yay!)
  partially implement GUI using messageBoxQueue. changed font family and size. add DropShadow to MessageBox.
  complete refactoring from previous commit, add GREY colour scheme
  fix VBox width issue, complete refactoring of dialogBox to messageBox
  refactor: rename DialogBox to MessageBox.add ColourScheme enum to take care of customisable colour schemes. configure getUserDialog and getDukeDialog to use background and text colour
  create CliDuke and GuiDuke classes to implement Duke interface, set default invocation of Duke from CLI to use CliDuke and GUI to use GuiDuke
  continue refactoring in earlier commit
  replace Ui class with Ui interface and Cli and Gui implementations, attempt to do the same for Duke
  refactor: organise methods in Duke class
  fix scrolling and resizing behaviour of MainWindow, add some JavaDoc
  refactor: move GUI-related classes to duke.gui, add title and application icon
  rectify MainWindow fit
  wrap TextField and Button in HBox
  change images to png (but still cannot get them to appear)
  manage to compile and run, opens GUI window
  ...
…-Tests-and-Assertions

* 'master' of https://github.com/nattanyz/duke:
  Set theme jekyll-theme-cayman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants